✨ Cleaner Condition Types & Reasons #1007

bentito · 2024-07-02T15:31:15Z

Description

Fixes: #996

Cleaner Condition Types & Reasons. See commit messages for coverage of this PR and its sync to the RFC recommendations. https://docs.google.com/document/d/1JWJxnDXM0X1JQ67ZIDx5j1ayb1d7ajrK_uSDDaz0G98

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

netlify · 2024-07-02T15:31:31Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`f678a7c`
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/669e4e46518dfc0008abd4fa
😎 Deploy Preview	https://deploy-preview-1007--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2024-07-02T16:00:22Z

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 72.93%. Comparing base (a0fca0d) to head (f678a7c).

Files	Patch %	Lines
...nternal/controllers/clusterextension_controller.go	50.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1007      +/-   ##
==========================================
+ Coverage   72.75%   72.93%   +0.17%     
==========================================
  Files          31       31              
  Lines        1883     1862      -21     
==========================================
- Hits         1370     1358      -12     
+ Misses        375      366       -9     
  Partials      138      138

Flag	Coverage Δ
e2e	`56.68% <0.00%> (+0.32%)`	⬆️
unit	`44.41% <50.00%> (-0.15%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bentito · 2024-07-03T15:44:33Z

NB: go-apidiff is an expected failing test as we're intentionally removing API items pre-GA.

api/v1alpha1/clusterextension_types.go

joelanford · 2024-07-03T17:38:01Z

internal/controllers/clusterextension_controller.go

-		setStatusUnpackPending(ext, unpackResult.Message)
-		setInstalledStatusConditionUnknown(ext, "installation has not been attempted as unpack is pending")
+		setStatusInstallFalseUnpackFailed(ext, unpackResult.Message)
+		setInstalledStatusConditionInstalledFalse(ext, "installation has not been attempted as unpack is pending")


Installed=False is not always correct here. If we have previously installed a bundle, and now the unpack of the next bundle is pending, we still have an installed bundle.

The installation status and the bundle unpack status are not related to each other.

okay, just setting the unpack as failed in 0306c9d

joelanford · 2024-07-03T17:39:46Z

internal/controllers/clusterextension_controller.go

@@ -273,13 +273,12 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp

 	switch unpackResult.State {
 	case rukpaksource.StatePending:
-		setStatusUnpackPending(ext, unpackResult.Message)
-		setInstalledStatusConditionUnknown(ext, "installation has not been attempted as unpack is pending")
+		setStatusInstallFalseUnpackFailed(ext, unpackResult.Message)


Unpack state is pending, but we're setting Unpacked=False, Failed?

Well this was changed to acknowledge that we've switched to direct image registry and that as far as opr-ctrl is concerned we can't really be in an Unknown state, we're either failed or not. Perhaps we should just not process StatePending here?

I'm not sure we should write this code with specific knowledge of the underlying Source implementation. In theory, the source could be switched out to a different implementation that can return StatePending. I've been tinkering with an async direct image client, so this is not entirely hypothetical.

More broadly, I wonder if we even need an Unpacked condition type? I don't see it in the diagram in the RFC. The only reason we are unpacking a bundle is so that we can install it. So can we rollup unpacking status into Progressing and/or Installed conditions?

That's farther than the RFC went, but I think it's in the same spirit. I think we can roll it into TypeInstalled. I think TypeProgressing would actually be adding a new type?

as noted above there is no TypeProgressing currently, if we want to eliminate TypeUnpacked in favor of TypeInstalled
@joelanford, these two helper methods encapsulate all the places changes would be needed:

func setStatusUnpackFailed(ext *ocv1alpha1.ClusterExtension, message string) { ext.Status.InstalledBundle = nil apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ Type: ocv1alpha1.TypeUnpacked, Status: metav1.ConditionFalse, Reason: ocv1alpha1.ReasonUnpackFailed, Message: message, ObservedGeneration: ext.GetGeneration(), }) } func setStatusUnpacked(ext *ocv1alpha1.ClusterExtension, message string) { apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ Type: ocv1alpha1.TypeUnpacked, Status: metav1.ConditionTrue, Reason: ocv1alpha1.ReasonUnpackSuccess, Message: message, ObservedGeneration: ext.GetGeneration(), }) }

So on the fail case we'd want TypeInstalled as ConditionUnknown and same on the success case. The reason would be the difference. But I wonder if the success case is a bit odd: TypeInstalled is Uknown and the reason is Unpack success?

I think TypeInstalled should literally just be based on: "is there a helm release secret with status deployed? if so, Installed=True. If not "Installed=False". If there was an error looking up helm release secrets (other than "not found"), then "Installed=Unknown".

Nothing else should play into it.

I created #1027 to capture this need. I'd like to keep this PR limited to the RFC covered items.

tmshort · 2024-07-12T14:25:14Z

@joelanford are you ok with this?

Signed-off-by: Brett Tofel <btofel@redhat.com>

tmshort

/lgtm

* Replace ReasonCreateDynamicWatchFailed w/ ReasonInstallationFailed Signed-off-by: Brett Tofel <btofel@redhat.com> * Remove unused ReasonInstallationSucceeded Signed-off-by: Brett Tofel <btofel@redhat.com> * Drop usage of ConditionUnknown on Pending Install Signed-off-by: Brett Tofel <btofel@redhat.com> * Remove ReasonUnpackPending/InstallationsStatusUnk Signed-off-by: Brett Tofel <btofel@redhat.com> * Update related TODO in ClusterExtension controller Signed-off-by: Brett Tofel <btofel@redhat.com> * Rm InstalledStatus->nil on upack & add comment Signed-off-by: Brett Tofel <btofel@redhat.com> * Align CRD for added Go doc on InstalledBundle Signed-off-by: Brett Tofel <btofel@redhat.com> * Move comment on InstalledBundle field Signed-off-by: Brett Tofel <btofel@redhat.com> * Commit manifest changes for godoc change Signed-off-by: Brett Tofel <btofel@redhat.com> * Decouple bundle unpacking and installed statuses Signed-off-by: Brett Tofel <btofel@redhat.com> * Remove unneeded unpack stage helper method Signed-off-by: Brett Tofel <btofel@redhat.com> --------- Signed-off-by: Brett Tofel <btofel@redhat.com>

bentito requested a review from a team as a code owner July 2, 2024 15:31

joelanford reviewed Jul 3, 2024

View reviewed changes

api/v1alpha1/clusterextension_types.go Outdated Show resolved Hide resolved

joelanford reviewed Jul 3, 2024

View reviewed changes

This was referenced Jul 9, 2024

Improve Error Handling in InstalledBundle Determination and Logic #1027

Open

✨ WIP Improved error handling around installedBundle and reconcile modularization #1030

Open

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 12, 2024

bentito added 11 commits July 22, 2024 07:57

Replace ReasonCreateDynamicWatchFailed w/ ReasonInstallationFailed

bda06aa

Signed-off-by: Brett Tofel <btofel@redhat.com>

Remove unused ReasonInstallationSucceeded

f3a024e

Signed-off-by: Brett Tofel <btofel@redhat.com>

Drop usage of ConditionUnknown on Pending Install

e0ba718

Signed-off-by: Brett Tofel <btofel@redhat.com>

Remove ReasonUnpackPending/InstallationsStatusUnk

b032582

Signed-off-by: Brett Tofel <btofel@redhat.com>

Update related TODO in ClusterExtension controller

8997c0c

Signed-off-by: Brett Tofel <btofel@redhat.com>

Rm InstalledStatus->nil on upack & add comment

2a20dfa

Signed-off-by: Brett Tofel <btofel@redhat.com>

Align CRD for added Go doc on InstalledBundle

0cff94c

Signed-off-by: Brett Tofel <btofel@redhat.com>

Move comment on InstalledBundle field

938e865

Signed-off-by: Brett Tofel <btofel@redhat.com>

Commit manifest changes for godoc change

4ec2c97

Signed-off-by: Brett Tofel <btofel@redhat.com>

Decouple bundle unpacking and installed statuses

b7607df

Signed-off-by: Brett Tofel <btofel@redhat.com>

Remove unneeded unpack stage helper method

f678a7c

Signed-off-by: Brett Tofel <btofel@redhat.com>

bentito force-pushed the cleaner-types-reasons branch from e5952e4 to f678a7c Compare July 22, 2024 12:19

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 22, 2024

tmshort approved these changes Jul 22, 2024

View reviewed changes

openshift-ci bot assigned tmshort Jul 22, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 22, 2024

tmshort added this pull request to the merge queue Jul 22, 2024

Merged via the queue into operator-framework:main with commit 58c5776 Jul 22, 2024
16 of 18 checks passed

skattoju mentioned this pull request Sep 9, 2024

🌱 Descriptive Upgrade Constraint Policy constants #1233

Merged

skattoju mentioned this pull request Sep 25, 2024

✨ Remove unpacked status #1309

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Cleaner Condition Types & Reasons #1007

✨ Cleaner Condition Types & Reasons #1007

bentito commented Jul 2, 2024 •

edited by tmshort

Loading

netlify bot commented Jul 2, 2024 •

edited

Loading

codecov bot commented Jul 2, 2024 •

edited

Loading

bentito commented Jul 3, 2024

joelanford Jul 3, 2024

bentito Jul 8, 2024 •

edited

Loading

joelanford Jul 3, 2024

bentito Jul 5, 2024

joelanford Jul 5, 2024

bentito Jul 8, 2024

bentito Jul 9, 2024

joelanford Jul 9, 2024

bentito Jul 9, 2024

tmshort commented Jul 12, 2024

tmshort left a comment

✨ Cleaner Condition Types & Reasons #1007

✨ Cleaner Condition Types & Reasons #1007

Conversation

bentito commented Jul 2, 2024 • edited by tmshort Loading

Description

Reviewer Checklist

netlify bot commented Jul 2, 2024 • edited Loading

✅ Deploy Preview for olmv1 ready!

codecov bot commented Jul 2, 2024 • edited Loading

Codecov Report

bentito commented Jul 3, 2024

Choose a reason for hiding this comment

bentito Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmshort commented Jul 12, 2024

tmshort left a comment

Choose a reason for hiding this comment

bentito commented Jul 2, 2024 •

edited by tmshort

Loading

netlify bot commented Jul 2, 2024 •

edited

Loading

codecov bot commented Jul 2, 2024 •

edited

Loading

bentito Jul 8, 2024 •

edited

Loading